Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(v2): autoprefix css #1605

Merged
merged 64 commits into from
Jul 20, 2019
Merged

feat(v2): autoprefix css #1605

merged 64 commits into from
Jul 20, 2019

Conversation

shakcho
Copy link
Contributor

@shakcho shakcho commented Jun 13, 2019

Motivation

Automatically add vendor prefix for CSS

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

In the root directory execute the following command

yarn 
yarn start

go to http://localhost:3000/docs/introduction
Open chrome devtools, we should see the position sticky should get vendor prefixed with
-webkit-sticky to work on Safari Browser as shown in the image below.
Screenshot 2019-06-19 at 1 53 51 AM

Try the URL in Safari browser, now the left sidebar should not scroll while scrolling.

Related PRs

I'm not aware of any

yangshun and others added 30 commits May 17, 2019 23:58
* misc(v2): clean up work

* misc(v2): rename components
* feat(v2): implement component overriding

* siteDir theme overriding should work for > 1 level directory

* fallback for essential component like Loading

* rename default -> classic
* feat(v2): easier plugin theme components override

* add context

* refactor again

* rename eject -> swizzle

* nits
* feat(v2): Algolia search plugin

* patch PR facebook#1440 (facebook#1441)

* alternative implementation

* typo

* refactor noop

* rename SearchAlgolia -> SearchBar

* changes.md
…k#1442)

* refactor(v2): move headerLinks -> themeConfig & rm dead code

* rm -rf dead code
* chore(v2): add flow setup

* nits

* fix

* add flow-typed

* ignore compiled library

* fix error

* fix typing

* fix module name mapper

* setup for @docusaurus/core

* dont try remove type without @flow

* fix can't find @docusaurus/utils

* fix test

* remove obscure relative paths

* more refactoring

* add typing for server/load/theme.js

* no need to ship .flow source
* feat(v2): meta description

* add description for blog as well

* fix non-descriptive text link

* remove font awesome

* switch front-matter -> gray-matter
* refactor(v2): blog data revamp

* fix(v2): fix incorrect blog total count

* misc: remove console.log

* feat(v2): export frontMatter as an object within MDX file (facebook#1451)

* refactor. Don't confuse metadata & frontmatter

* export frontMatter in content itself

* nits

* nits name

* dont truncate first four lines in blog
* feat(v2): blog tags

* feat(v2): blog tags
* fix: right TOC should not strip special chars

* nits
* fix(v2): handle non existent blog, docs, pages

* nits
* feat(v2): list blog tags on posts

* fix date handling on blog header

* fix console log error due to non unique key
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 13, 2019
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 13, 2019

Deploy preview for docusaurus-2 ready!

Built with commit b708cb7

https://deploy-preview-1605--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit 5763760

https://deploy-preview-1605--docusaurus-preview.netlify.com

@shakcho shakcho changed the title Css autoprefix fix: Sticky sidebars don’t stick in desktop safari Jun 13, 2019
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 13, 2019

Deploy preview for docusaurus-preview ready!

Built with commit b708cb7

https://deploy-preview-1605--docusaurus-preview.netlify.com

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the attempt :)

  1. When using postcss-loader in webpack with css-loader, u need to set importLoader in css-loader. Try to read more on css-loader as well
  2. for browserslist, user doesnt always have the configuration. Sometimes everyone want to compile to all browser, did you make sure it works and has a default ?
  3. Please provide a test plan that makes sure it fixes the problem, what kind of difference adding this postcss loader add. “Run automated test cases” as a test plan isn’t a good practice.
  4. While CRA config can be taken as inspiration, CRA is not server side rendered. Did you make sure it works on SSR. ?

CRA also doesnt allow customizing postcss options (which made people had to create CRA-rewired and all hacky tools out there) or they had to eject.

Can we do this for docusaurus without ejecting ?
5. Do we really need stage 3 feature and postcss-flexbugs-fixes ?

This is a good start, thank you

Please provide a strong test plan,

@shakcho
Copy link
Contributor Author

shakcho commented Jun 14, 2019

@endiliey Thanks for your support. I will read about all the things you mentioned. And do the necessary changes. If needed I will ask you for help. Thanks

@shakcho shakcho changed the title fix: Sticky sidebars don’t stick in desktop safari wip: Sticky sidebars don’t stick in desktop safari Jun 14, 2019
@shakcho
Copy link
Contributor Author

shakcho commented Jun 18, 2019

@endiliey Updated the pull request
But I still have doubt about how to solve the below mentioned issue.

  1. Allow users to update browserslist
  • My idea is to, let the project use browserslist conf from the package.json itself. If someone wants to update the browserslist, they can use browserslist-config-mycompany . Let me know if you have something else in mind.
  1. I don't know how to setup SSR, so unable to test this, please help me to test this.

@shakcho
Copy link
Contributor Author

shakcho commented Jun 26, 2019

@endiliey @yangshun please check this

@shakcho shakcho changed the title wip: Sticky sidebars don’t stick in desktop safari fix: Sticky sidebars don’t stick in desktop safari Jul 2, 2019
@endiliey endiliey requested a review from wgao19 July 13, 2019 16:53
@endiliey
Copy link
Contributor

dont have a safari to test it out. overall lgtm but not sure if we want to compile postcss just for autoprefixer

cc @yangshun @wgao19

@endiliey endiliey changed the title fix: Sticky sidebars don’t stick in desktop safari fix(v2): Sticky sidebars don’t stick in desktop safari Jul 14, 2019
@wgao19
Copy link
Contributor

wgao19 commented Jul 19, 2019

i'd go for it because autoprefixing css counts as a chore that people will most likely have to deal with or may not even be aware that their projects already have it set up

@endiliey endiliey changed the title fix(v2): Sticky sidebars don’t stick in desktop safari feat(v2): autoprefix css Jul 19, 2019
Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls rebase or merge conflict

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yangshun @wgao19 lgtm lgtyall ? :p

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@yangshun yangshun merged commit 1a5aed4 into facebook:master Jul 20, 2019
@endiliey
Copy link
Contributor

Thanks a lot @shakcho

Sorry for the delay, was sick

@shakcho
Copy link
Contributor Author

shakcho commented Jul 20, 2019

@endiliey @yangshun @wgao19 Thanks for the opportunity to allow me to work on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants